Skip to content

Fix crash when natives send ships to Europe (#1039) - #1221

Open
acato wants to merge 13 commits into
We-the-People-civ4col-mod:developfrom
acato:fix/native-europe-crash
Open

Fix crash when natives send ships to Europe (#1039)#1221
acato wants to merge 13 commits into
We-the-People-civ4col-mod:developfrom
acato:fix/native-europe-crash

Conversation

@acato

@acato acato commented Mar 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Natives can acquire ocean-going ships (e.g. via gifting a Slave Barque). The ship's AI then sails to Europe, but natives have no Europe dock — the game crashes when accessing non-existent dock units.
  • Added isNative() early-return checks to canCrossOcean(), canSailToAfrica(), and canSailToPortRoyal() so native-owned ships are blocked from leaving the map entirely.

Fixes #1039

Test plan

  • Build Assert DLL and verify no compilation errors
  • Gift an ocean-capable ship to a native tribe via World Builder
  • Advance turns and confirm no crash (ship should remain on the map)
  • Verify European players can still sail to Europe/Africa/Port Royal normally

🤖 Generated with Claude Code

acato and others added 12 commits March 13, 2026 19:46
Extends the trade route system to support Africa and Port Royal as
off-map trade locations alongside Europe. Adds sentinel IDs, the
isOffMapTradeLocation() helper, route validation, AI evaluation,
Python UI support in the Trade Routes Advisor, and canSailToAfrica/
canSailToPortRoyal Python bindings.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Custom MinUnit-derived C++03 test framework with 35 tests across 6 suites
covering EnumMap, JustInTimeArray, Coordinates, CvTradeRoute sentinels,
and CvIdVector. Tests run at startup in Assert/Debug builds after XML load,
logging results to Logs\WTPTests.log.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
C++ tests (10) validate XML cross-references at startup: profession
yields, building classes, unit professions, father CivEffects/categories,
yield costs, equipment amounts, terrain yields, and trade sentinels.

Perl scripts: test_determinism.pl detects non-deterministic calls
(rand/srand/time/GetTickCount) outside allowed files; test_text_keys.pl
verifies TXT_KEY references in data XML have matching text definitions.
Both run as part of the test_DllExport build target.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 3: MemoryStream (in-memory FDataStreamBase) with 23 round-trip tests
covering primitives, strings, IDInfo, JustInTimeArray, and enum types.

Fix XMLIntegrity test: allow NO_YIELD (-1) in profession yield lists, as
the game code explicitly checks and skips it. Fix int-to-enum casts for
VC++ 2003 strict type checking.

Fix Savegame test: JustInTimeArray::Read() always allocates, even for
empty arrays — removed incorrect !isAllocated() assertion.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Set GAME_MODS_PATH in Makefile.settings to automatically xcopy the mod
to the game's Mods directory as a post-build step.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ships with automated trade routes to off-map destinations (Africa, Port
Royal) would get stuck upon arrival or lose their route assignments when
crossing the ocean. Four root causes fixed:

- AI_update() now intercepts units in Port Royal (was only Europe/Africa)
- AI_europeUpdate() handles AUTOMATE_TRANSPORT_ROUTES ships: sells cargo
  and crosses back automatically (previously only AUTOMATE_FULL was handled)
- AI_transportMoveRoutes() routes to correct destination via new helper
  AI_sailToOffMapTradeDestination() instead of always sailing to Europe
- setUnitTravelState() preserves automation and trade route assignments
  across group splits during ocean travel

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 4: CvRandom determinism tests — seed reproducibility, range bounds,
peek non-advancement, float output, and state round-trips (576 checks).

Phase 5: Python-side test suite (WTPTests.py) validating the DLL-Python
bridge and XML data consistency — profession yields, building classes,
unit professions, father categories, terrain yields, civilization info.
Runnable from debug console: import WTPTests; WTPTests.runAllTests()

Also fixes WTPTests.log double-nesting (gDLL->logMsg already writes to
the Logs/ directory).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
setCityYieldModifierString was missing the YIELD_LAW exclusion when
applying the rebel yield modifier, causing a mismatch with
getBaseYieldRateModifier which correctly excludes it.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
EventTriggeredData::setRandomNumbers() creates a RandomContainer for
each event in the trigger, generates a random number via getSorenRandNum,
but never actually stores the container in m_RandomNumbers. The
push_back call was missing, so m_RandomNumbers remained empty after
the function completed.

This caused getRandomNumber() and getRandomNumberForIndex() to always
return 0 (the fallback default), which had two consequences:

1. Any Python PythonCanDo callback using getRandomNumberForIndex()
   for probability checks would always see 0. For example,
   canTriggerVolcanoDormant1() checks "getRandomNumberForIndex(0) < 250"
   which was always true (0 < 250), making the volcano dormant event
   fire 100% of the time instead of the intended ~25%.

2. The DLL-side TriggerChance fastpath in CvPlayer::canDoEvent()
   (line 14762) uses getRandomNumber(eEvent) to check event probability
   thresholds. With the vector always empty, this also always returned 0,
   bypassing intended probability gates for events validated through
   that code path.

The fix adds the missing m_RandomNumbers.push_back(container) so that
generated random numbers are actually persisted in the vector and
available to both Python and C++ callers.

Related: We-the-People-civ4col-mod#1205 (CvRandomInterfaceEvent tuple index out of range)
The primary cause of We-the-People-civ4col-mod#1205 was an argsList index mismatch fixed in
38b7518, but this bug compounded the issue by making the random
number check in canTriggerVolcanoDormant1 a no-op.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
In CvPlayer::initTriggeredData(), after a PythonCanDo callback returns
successfully, the code reconstructs a Coordinates object from the
trigger data (since Python may have modified it). However, line 14416
passed m_iPlotX as both the X and Y arguments:

  Coordinates coord(pTriggerData->m_iPlotX, pTriggerData->m_iPlotX);

This meant pPlot was resolved to the wrong tile whenever the Python
callback modified the trigger's plot coordinates and X != Y. The
resolved plot would be at (X, X) instead of (X, Y), causing the
event to target the wrong map location for any subsequent logic
that uses pPlot (text generation, world news, event application).

Events where PythonCanDo does not modify coordinates would be
unaffected since pPlot was already correctly set before the callback.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Natives have no Europe dock, so if they acquire an ocean-going ship
(e.g. via gifting) and it sails to Europe, the game crashes accessing
non-existent dock units. Add isNative() checks to canCrossOcean,
canSailToAfrica, and canSailToPortRoyal to block the trip entirely.

Fixes We-the-People-civ4col-mod#1039

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread Project Files/DLLSources/CvUnit.cpp Outdated
bool CvUnit::canCrossOcean(const CvPlot* pPlot, UnitTravelStates eNewState) const
{
// Natives have no Europe/Africa/Port Royal — prevent crash when they acquire ocean ships
if (isNative())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would greatly prefer checking whether the player either 1) has a parent, and is therefore a European player, or 2) is a king. See CvPlayer::isColonialNation() and the related identity functions.

Kings do not really interact with Europe in the normal sense. Their ships are spawned there after independence is declared, but the king does not currently trade with Europe after that. Even so, treating the king as conceptually connected to Europe is still the correct abstraction, especially if we later implement this feature for kings as well.

Another advantage of not checking directly whether the player is native is that this also covers other player types, such as wild animals, i.e. the barbarian black team.

I am not going to add the same comment to the other two functions, but they should obviously use the same check(s).

A separate question is how the native player got hold of the Slave Barque in the first place. That is worth investigating and locking down, since it makes little sense given the AI's capabilities and the general oddity of natives having an ocean-going vessel. In short I think it should be banned, but that is a topic for another ticket.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are 200% right. I had not thought about barbarians and wild animals as special cases. I updated the PR with the check you suggested.

Replace isNative() check with a positive identity check: only players
with a parent (colonial nations) or kings (isEurope) can sail to
Europe/Africa/Port Royal. This also covers wild animals and other
non-standard player types.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Natives crash the game by sending ships to Europe if they get a ship, which is able to leave the map

2 participants